docs: correct an inverted numeric-key example and a stale test docblock - #668
Conversation
Two nits from Wilco's approving review on #656, which merged before they were addressed. Comments only - no shipped behaviour changes. THE NUMERIC-STRING KEY EXAMPLE WAS BACKWARDS. normaliseUserIds() explained why a keyed set is unsafe for user ids and then gave the wrong example: it claimed "0123" would come back from array_keys() as 123. PHP coerces an array key that is a CANONICAL decimal integer string, so it is "123" and "-7" that become ints, while "0123", "007" and "1e3" stay strings. Verified rather than reasoned: "123" -> integer "0123" -> string "-7" -> integer "007" -> string, "1e3" -> string The conclusion held - a keyed set is still the wrong tool here - but a wrong example is worse than none, because it teaches the opposite rule to the next reader. Corrected, and it now notes the sharper reason: coercion depends on the SHAPE of the id, so some ids would survive and others would not, which is worse than breaking uniformly. The same error appeared in the PR discussion; a correction is posted there. THE TEST DOCBLOCK STILL DESCRIBED THE OLD CONTRACT. testRecipientCertificatesPreservesOrderAndDeduplicates said the caller zips the response against the request and that order is "part of the contract" - the claim that was removed from the endpoint's own documentation in the same PR. The test still asserts order, which is right; it just is not a guarantee anyone may rely on. Assisted-by: ClaudeCode:claude-opus-5
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 111/111 | |||
| npm | ✅ | ✅ 543/543 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-08 14:16 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (Quick mode)
No findings. Every claim in the description checks out.
The corrected coercion rule is right — and complete. I verified it empirically rather than from memory (PHP 8.4.18), against every string the comment names plus eight it doesn't:
'123' → int 123 '0' → int 0
'-7' → int -7 '-0' → string '-0'
'0123' → string ' 1' / '1 ' / '+1' → string
'007' → string '9223372036854775807' → int (PHP_INT_MAX)
'1e3' → string '9223372036854775808' → string (overflows, stays string)
'1.0' → string
Every one is consistent with "canonical decimal integer string". The unnamed edge cases don't contradict the rule, so the comment is safe to rely on — which is the whole point of fixing it.
No shipped behaviour change, as claimed: the diff touches only comment and docblock lines.
The test docblock now matches reality. It no longer claims order is a contract, while the test still asserts order — correctly described as a convenience rather than a guarantee. That agrees with the endpoint's own docblock at ShareController.php:455-457 ("First-seen order is preserved as a convenience, but it is NOT a positional contract… Callers correlate by userId"), so the two no longer disagree.
Completeness checked: grep -rn "part of the contract" lib/ tests/ docs/ openspec/ src/ and a numeric-coercion sweep turn up no surviving copies of either wrong claim elsewhere.
Thanks for circling back on these after #656 merged — a wrong example really is worse than no example, and the sharper reasoning about coercion depending on the shape of the id is a genuine improvement over what I originally flagged.
3 participants
Follow-up to the two nits in @WilcoLouwerse's approving review on #656, which merged before they were addressed. Comments and one test docblock only — no shipped behaviour changes.
The numeric-string key example was backwards
normaliseUserIds()explains why a keyed set is the wrong tool for deduplicating user ids, and then gave the wrong example — it claimed"0123"would come back fromarray_keys()as123. Wilco caught it. PHP coerces an array key only when it is a canonical decimal integer string:Verified by running it rather than reasoning about it.
The conclusion still stood — a keyed set really is unsafe here — but a wrong example is worse than no example, because it teaches the next reader the opposite rule. The corrected comment also states the sharper reason: coercion depends on the shape of the id, so some ids would survive intact and others would silently change type. That's worse than breaking uniformly, because it wouldn't show up in testing with the wrong sample of ids.
I repeated the same error in the #656 discussion, so a correction is posted on that thread too.
Stale test docblock
testRecipientCertificatesPreservesOrderAndDeduplicatesstill said the caller zips the response against the request and that order is "part of the contract, not an accident" — the exact claim removed from the endpoint's own documentation in the same PR, after Wilco pointed out it doesn't survive deduplication. The test still asserts order, which is correct; it just isn't a guarantee anyone may depend on.1192 tests pass, phpcs 0 errors, phpmd clean.
🤖 AI disclosure: prepared with Claude Code (Opus 5). Commits carry
Assisted-by:trailers. Reviewed and submitted by @rjzondervan.